feat(icon-button): Add SVG support #3310

williamernest · 2018-08-07T22:31:25Z

fixes: #3277 , resolves #3154, resolves #3107
BREAKING CHANGE: Removes the previous data attributes and no longer dynamically changes the label. Allows developers to add both elements to the button, with one indicated as the on state by using a data-toggle-on attribute. State is now changed by adding/removing the mdc-icon-button--on class to the mdc-icon-button element. All icon elements should have the mdc-icon-button__icon class.

mdc-web-bot · 2018-08-07T22:40:36Z

🤖 Beep boop!

Screenshot test report 🚦

1 screenshot changed from master on commit fe86346:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/08/07/22_39_51_848/report/report.html

Details

1 Changed:

spec/mdc-card/classes/baseline.html

codecov-io · 2018-08-07T22:43:12Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a284aae). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #3310   +/-   ##
=========================================
  Coverage          ?   98.06%           
=========================================
  Files             ?      120           
  Lines             ?     5155           
  Branches          ?      644           
=========================================
  Hits              ?     5055           
  Misses            ?      100           
  Partials          ?        0

Impacted Files	Coverage Δ
packages/mdc-icon-button/constants.js	`100% <ø> (ø)`
packages/mdc-icon-button/foundation.js	`100% <100%> (ø)`
packages/mdc-icon-button/index.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a284aae...0d1146c. Read the comment docs.

mdc-web-bot · 2018-08-07T22:56:31Z

🤖 Beep boop!

Screenshot test report 🚦

1 screenshot changed from master on commit b18cfba:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/08/07/22_55_48_924/report/report.html

Details

1 Changed:

spec/mdc-card/classes/baseline.html

mdc-web-bot · 2018-08-07T23:09:39Z

🤖 Beep boop!

Screenshot test report 🚦

1 screenshot changed from master on commit ff8e432:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/08/07/23_08_52_983/report/report.html

Details

1 Changed:

spec/mdc-card/classes/baseline.html

kfranqueiro · 2018-08-08T17:56:11Z

demos/card.html

-                 data-toggle-off-content="favorite_border"
-                 data-toggle-off-label="Add to favorites">favorite_border</button>
+                 title="Add to favorites" data-demo-toggle>
+                <i class="material-icons mdc-icon-button__icon" data-toggle-on>favorite</i>


Given that we need to select on this data attribute in CSS, it might make sense to make it a class instead (e.g. mdc-icon-button__icon--on?) - attribute selectors perform a lot slower in IE and Edge.

kfranqueiro · 2018-08-08T18:33:49Z

packages/mdc-icon-button/README.md

+Note the use of `data-toggle-on` attribute in the above examples. This attribute indicates which element is the
+on element. When the `mdc-icon-button--on` class is present, the element with the `data-toggle-on` attribute
+will be shown and the other element will be hidden. When the `mdc-icon-button--on` class is not present, the element
+with the `data-toggle-on` attribute will be hidden and the other element will beshown. This is what allows


beshown -> be shown

kfranqueiro · 2018-08-08T18:35:03Z

packages/mdc-icon-button/README.md

+on element. When the `mdc-icon-button--on` class is present, the element with the `data-toggle-on` attribute
+will be shown and the other element will be hidden. When the `mdc-icon-button--on` class is not present, the element
+with the `data-toggle-on` attribute will be hidden and the other element will beshown. This is what allows
+MDCIconButtonToggle to be so flexible. 


"This is what allows MDCIconButtonToggle to be so flexible." -> "This structure allows MDCIconButton to be flexible enough to support both icon fonts and images/SVGs."

kfranqueiro · 2018-08-08T18:39:47Z

packages/mdc-icon-button/README.md

@@ -140,6 +157,7 @@ cannot be disabled. Disabled icon buttons cannot be interacted with and have no
 CSS Class | Description
 --- | ---
 `mdc-icon-button` | Mandatory.
+`mdc-icon-button--on` | Used to indicate the toggle button icon that is the not-selected option.


This doesn't sound right... This class is applied to the root element when the toggle should be in the "on" state, right?

kfranqueiro · 2018-08-08T18:40:24Z

packages/mdc-icon-button/README.md

-`addClass(className: string) => void` | Adds a class to the root element, or the inner icon element.
-`removeClass(className: string) => void` | Removes a class from the root element, or the inner icon element.
-`setText(text: string) => void` | Sets the text content of the root element, or the inner icon element.
+`addClass(className: string) => void` | Adds a class to a icon element.


Why were these docs changed? Doesn't this still apply to the root element?

kfranqueiro · 2018-08-08T18:43:59Z

packages/mdc-icon-button/foundation.js

-    if (label) {
-      this.adapter_.setAttr(ARIA_LABEL, label);
-    }
+    this.adapter_.setAttr(strings.ARIA_PRESSED, `${this.isOn()}`);


this.isOn() -> isOn?

kfranqueiro · 2018-08-08T18:44:34Z

packages/mdc-icon-button/foundation.js

-    if (classToRemove) {
-      this.adapter_.removeClass(classToRemove);
+    this.on_ = isOn;
+    if (this.isOn()) {


this.isOn() -> isOn? We already have the variable right here.

kfranqueiro · 2018-08-08T18:45:34Z

test/screenshot/golden.json

@@ -157,7 +157,7 @@
    "screenshots": {
      "desktop_windows_chrome@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/advorak/2018/07/31/04_55_54_478/spec/mdc-card/classes/baseline.html.windows_chrome_67.png",
      "desktop_windows_edge@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/advorak/2018/07/31/04_55_54_478/spec/mdc-card/classes/baseline.html.windows_edge_17.png",
-      "desktop_windows_firefox@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/advorak/2018/07/31/04_55_54_478/spec/mdc-card/classes/baseline.html.windows_firefox_61.png",
+      "desktop_windows_firefox@latest": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/08/07/22_51_57_136/spec/mdc-card/classes/baseline.html.windows_firefox_61.png",


This seems weird...is this diff consistent, and if so do we have any possible explanation?

Yes, it's consistent. This was a result of me accepting screenshot diffs from master, not from within this branch.

kfranqueiro · 2018-08-08T18:46:50Z

test/unit/mdc-icon-button/foundation.test.js

@@ -22,6 +22,7 @@ import {verifyDefaultAdapter} from '../helpers/foundation';
 import MDCIconButtonToggleFoundation from '../../../packages/mdc-icon-button/foundation';

 const {strings} = MDCIconButtonToggleFoundation;
+const {cssClasses} = MDCIconButtonToggleFoundation;


Change above line to const {cssClasses, strings} = ... and you don't need this line

kfranqueiro · 2018-08-08T18:51:09Z

test/unit/mdc-icon-button/mdc-icon-button-toggle.test.js

-});
-
-test('#adapter.addClass adds a class to the inner icon element when used', () => {
-  const {root, component} = setupTest({useInnerIconElement: true});


We seem to never use useInnerIconElement or tabIndex in setupTest anymore.

Are we losing anything by no longer supporting the inner icon element? I'm inclined to think that's mitigated by having separate DOM for each state now.

mdc-web-bot · 2018-08-08T22:22:56Z

🤖 Beep boop!

Screenshot test report 🚦

5 screenshots changed from master on commit ceaa86a:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/08/08/22_22_10_318/report/report.html

Details

1 Changed:

spec/mdc-card/classes/baseline.html

4 Added:

spec/mdc-icon-button/classes/baseline-icon-button-toggle.html

kfranqueiro

We don't seem to have a foundation test for the isOn method.

kfranqueiro · 2018-08-09T15:28:10Z

packages/mdc-icon-button/constants.js

-  DATA_TOGGLE_OFF_LABEL: 'data-toggle-off-label',
-  DATA_TOGGLE_OFF_CONTENT: 'data-toggle-off-content',
-  DATA_TOGGLE_OFF_CLASS: 'data-toggle-off-class',
+  DATA_TOGGLE_ON_ATTR: 'data-toggle-on',


This is no longer used since it's now a class

kfranqueiro · 2018-08-09T15:28:23Z

packages/mdc-icon-button/constants.js

@@ -18,16 +18,12 @@
 /** @enum {string} */
 const cssClasses = {
  ROOT: 'mdc-icon-button',
+  ICON_BUTTON_ON_CLASS: 'mdc-icon-button--on',


_CLASS seems redundant within the cssClasses hash... we could potentially even shorten this to just ON since it's a modifier class for the root element?

kfranqueiro · 2018-08-09T15:36:03Z

packages/mdc-icon-button/mdc-icon-button.scss

+.mdc-icon-button__icon {
+  display: inline-block;
+
+  // stylelint-disable-next-line plugin/selector-bem-pattern


Just leaving a note here for posterity, that although this needs to do things that aren't entirely in line with BEM, we're significantly simplifying the JS implementation (including adapter) by toggling a single class on the root rather than needing to add a class to one child and remove it from the other every time the icon button is toggled.

kfranqueiro · 2018-08-09T15:37:48Z

test/screenshot/spec/mdc-icon-button/classes/baseline-icon-button-toggle.html

+<html lang="en">
+  <head>
+    <meta charset="utf-8">
+    <title>Baseline Icon Button - MDC Web Screenshot Test</title>


Add "Toggle" to this title

mdc-web-bot · 2018-08-09T20:29:16Z

🤖 Beep boop!

Screenshot test report 🚦

5 screenshots changed from master on commit 0d1146c:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/08/09/20_27_22_409/report/report.html

Details

1 Changed:

spec/mdc-card/classes/baseline.html

4 Added:

spec/mdc-icon-button/classes/baseline-icon-button-toggle.html

williamernest and others added 2 commits August 7, 2018 15:24

feat(icon-button): Add svg support

c4799bc

Merge branch 'master' into feat/icon-button/svg-support

fe86346

feat(icon-button): Add img demo to demo/readme. Update golden

b18cfba

feat(icon-button): Update golden

ff8e432

kfranqueiro mentioned this pull request Aug 8, 2018

Update instances of Icon Button for 0.39 material-components/material-components-web-catalog#182

Closed

kfranqueiro suggested changes Aug 8, 2018

View reviewed changes

feat(icon-button): Update for comments

ceaa86a

kfranqueiro suggested changes Aug 9, 2018

View reviewed changes

williamernest and others added 3 commits August 9, 2018 10:32

feat(icon-button): Update for comments

49eba69

feat(icon-button): Update tests

4cb157c

Merge branch 'master' into feat/icon-button/svg-support

0d1146c

kfranqueiro approved these changes Aug 9, 2018

View reviewed changes

williamernest merged commit 25fa51e into master Aug 9, 2018

williamernest deleted the feat/icon-button/svg-support branch August 9, 2018 21:09

kfranqueiro mentioned this pull request Aug 15, 2018

Support non-font (SVG) icons in the toggle button. #3364

Closed

jamesmfriedman mentioned this pull request Aug 23, 2018

MDC 38/39 rmwc/rmwc#281

Closed

48 tasks

trimox mentioned this pull request Aug 23, 2018

Implement redesigned MDC Icon Button trimox/angular-mdc-web#1245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(icon-button): Add SVG support #3310

feat(icon-button): Add SVG support #3310

williamernest commented Aug 7, 2018 •

edited

Loading

mdc-web-bot commented Aug 7, 2018

1 Changed:

codecov-io commented Aug 7, 2018 •

edited

Loading

mdc-web-bot commented Aug 7, 2018

1 Changed:

mdc-web-bot commented Aug 7, 2018

1 Changed:

kfranqueiro Aug 8, 2018

kfranqueiro Aug 8, 2018

kfranqueiro Aug 8, 2018

kfranqueiro Aug 8, 2018

kfranqueiro Aug 8, 2018

kfranqueiro Aug 8, 2018

kfranqueiro Aug 8, 2018

kfranqueiro Aug 8, 2018

williamernest Aug 8, 2018

kfranqueiro Aug 8, 2018

kfranqueiro Aug 8, 2018

mdc-web-bot commented Aug 8, 2018

1 Changed:

4 Added:

kfranqueiro left a comment

kfranqueiro Aug 9, 2018

kfranqueiro Aug 9, 2018

kfranqueiro Aug 9, 2018

kfranqueiro Aug 9, 2018

mdc-web-bot commented Aug 9, 2018

1 Changed:

4 Added:

feat(icon-button): Add SVG support #3310

feat(icon-button): Add SVG support #3310

Conversation

williamernest commented Aug 7, 2018 • edited Loading

mdc-web-bot commented Aug 7, 2018

Screenshot test report 🚦

1 Changed:

codecov-io commented Aug 7, 2018 • edited Loading

Codecov Report

mdc-web-bot commented Aug 7, 2018

Screenshot test report 🚦

1 Changed:

mdc-web-bot commented Aug 7, 2018

Screenshot test report 🚦

1 Changed:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Aug 8, 2018

Screenshot test report 🚦

1 Changed:

4 Added:

kfranqueiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Aug 9, 2018

Screenshot test report 🚦

1 Changed:

4 Added:

williamernest commented Aug 7, 2018 •

edited

Loading

codecov-io commented Aug 7, 2018 •

edited

Loading